-
Notifications
You must be signed in to change notification settings - Fork 213
Savetxt unit #1085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Savetxt unit #1085
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1085 +/- ##
==========================================
- Coverage 68.62% 68.61% -0.01%
==========================================
Files 394 394
Lines 12730 12734 +4
Branches 1376 1376
==========================================
+ Hits 8736 8738 +2
- Misses 3994 3996 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for this @fiolj. Would you mind reverting the changes not related to the implementation? (styling) There are too many and it renders difficult to read through the PR. You can check the style_guide for info https://github.com/fortran-lang/stdlib/blob/master/STYLE_GUIDE.md One thing, white spaces in-between parentheses and an intrinsic function are not recommended ( |
|
Thanks @jalvesz, I've fixed the formatting |
jvdp1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @fiolj . Here are some suggestions
doc/specs/stdlib_io.md
Outdated
| `array`: Shall be a rank-2 array of type `real`, `complex` or `integer`. | ||
|
|
||
| `delimiter` (optional): Shall be a character expression of length 1 that contains the delimiter used to separate the columns. The default is `' '`. | ||
| `filename or unit`: Shall be either a character expression containing the name of the file or an integer containing the unit of an already open file, that will contain the 2D `array`. Setting the two of them shall give an error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `filename or unit`: Shall be either a character expression containing the name of the file or an integer containing the unit of an already open file, that will contain the 2D `array`. Setting the two of them shall give an error | |
| `filename or unit`: Shall be either a character expression containing the name of the file or an integer containing the unit of an already open file, that will contain the 2D `array`. Setting the two of them shall give an error. |
src/stdlib_io.fypp
Outdated
| use stdlib_optval, only: optval | ||
| use stdlib_ascii, only: is_blank | ||
| use stdlib_string_type, only : string_type, assignment(=), move | ||
| ! use stdlib_strings, only: replace_all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ! use stdlib_strings, only: replace_all |
src/stdlib_io.fypp
Outdated
| character(len=*), intent(in), optional :: delimiter ! Column delimiter. Default is a space. | ||
| character(len=*), intent(in), optional :: header !< If present, text to write before data. | ||
| character(len=*), intent(in), optional :: footer !< If present, text to write after data. | ||
| character(len=1), intent(in), optional :: comments !< Comment character. Default "#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it in all languages only one character? (furthermore, I don't think that there will be a check if the lenght is mentioned)
| character(len=1), intent(in), optional :: comments !< Comment character. Default "#" | |
| character(len=*), intent(in), optional :: comments !< Comment character. Default "#" |
src/stdlib_io.fypp
Outdated
| character(len=3) :: delim_str | ||
| character(len=:), allocatable :: default_fmt | ||
| character(len=:), allocatable :: fmt_ | ||
| character(len=1), allocatable :: comments_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| character(len=1), allocatable :: comments_ | |
| character(len=:), allocatable :: comments_ |
src/stdlib_io.fypp
Outdated
| pure function prepend(Sin, comment) result(Sout) | ||
| character(len=*), intent(in) :: Sin | ||
| character(len=:), allocatable :: Sout | ||
| character(len=1), intent(in) :: comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| character(len=1), intent(in) :: comment | |
| character(len=*), intent(in) :: comment |
src/stdlib_io.fypp
Outdated
| character(len=*), intent(in) :: Sin | ||
| character(len=:), allocatable :: Sout | ||
| character(len=1), intent(in) :: comment | ||
| character(len=3) :: com_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep some flexibility:
| character(len=3) :: com_ | |
| character(len=:), allocatable :: com_ |
|
|
||
| s = open(filename, "w") | ||
| !! Write the header if non-empty | ||
| ! prepend function may be replaced by use of replace_all but currently stdlib_strings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably something that we should solve at a later stage.
doc/specs/stdlib_io.md
Outdated
|
|
||
| `footer`: (optional) Shall be a character expression that will be written at the end of the file. | ||
|
|
||
| `comments`: (optional) : Shall be a character expression of length 1 that will be prepended to the ``header`` and ``footer`` strings to mark them as comments. Default: `# `. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep the lenght of comments undefined? (E.g., what about is a user would like to pass !# as comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree.
|
Reviewing the arguments of Currently, we have the order of |
As |
Thanks, I agree. Last uploaded commit has the new order and some small cleanup of the code. I think that it this version is close to apt to be merged, except for the codecov failings (which I have no idea what they mean). |
It is related to the examples that are not covered by these pipelines. I think that @jalvesz got a similar issue with another PR. |
|
The fails with codecov are valid in my opinion. In #1074 we avoided codecov from analyzing the examples as a metric for coverage. But in this case, it is a valid fail because there are API changes which are not being tested. It is not enough to just add an example to ensure correctness. |
I am confused. This shows that some lines of
I agree with you. |
Co-authored-by: Jeremie Vandenplas <[email protected]>
@fiolj could you update the tests to cover the different changes in the API? |
Yes. I've commited some tests. |
I'm also confused now, codecov is supposed to ignore files in the examples directory after the PR #1074 ... I would say for the moment to ignore this false error. |
This PR aims to add optional arguments to
savetxt, that behave similar to numpy's savetxt.This is associated with Issue 263 and this discussion thread.
It add the possibility of supplying the unit of an open file instead of a filename (which could be used for
output_unitfor instance)This implementation is quite simple. The main changes are: